-
Notifications
You must be signed in to change notification settings - Fork 2.4k
Jira Integration: Allow configuring issue update via parameter #4621
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Jira Integration: Allow configuring issue update via parameter #4621
Conversation
|
this is also important due to upcoming rate-limiting https://developer.atlassian.com/cloud/jira/platform/rate-limiting/ |
tbh this sounds to me like it will be mandatory to have some kind of back off mechanism to avoid running into these enforced rate limits? |
|
@holger-waschke i think this is already implemented alertmanager/notify/jira/jira.go Line 63 in f7ff687
|
|
I'm with you, but this handles every alarm separate. I think generally an optional circuit-breaker could solve this.
Maybe we should discuss this in an issue. |
|
I like this idea, but I'm wondering if we should have a more generic implementation. I'm less familiar with the Jira integration than I should be, honestly. Are there any other fields which we might want to avoid updating on repeat notifications? I'm wondering if we should allow fields to have sub-keys that indicate if we should update them or not. I also think the discussion about rate-limiting is important enough to move to a dedicated issue. It's really a problem for any receiver, so it's something we should ideally solve generically. |
we´re using this Jira API Endpoint to update existing issues In the request body we send the fields we would like to update. which are the same as creating a new issue so basically everything. |
That makes sense to me... The thing I'm worried about is that we'll end up having multiple What do you think about introducing a new This would result in config like: We could then reuse the type for cc @siavashs and @grobinson-grafana since they might have a different opinion. |
91101c8 to
c792c12
Compare
|
@Spaceman1701 The jira_test required a fair amount of refactoring to accommodate this, but it feels justified. |
c81ed9c to
9da06dc
Compare
Spaceman1701
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for making those changes! I like the implementation, this is looking good to me.
Last thing is to make sure the docs reflect this change.
notify/jira/types.go
Outdated
| Project *issueProject `json:"project,omitempty"` | ||
| Resolution *idNameValue `json:"resolution,omitempty"` | ||
| Summary string `json:"summary"` | ||
| Summary any `json:"summary"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why did this change? I see that Description is also any, so I think this is fine. Just answering here in github is enough that someone will be able to learn the answer if they come looking for it in the future 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any is an alias for interface{} and we´re using it in combination with the MarshalJSON function
// MarshalJSON merges the struct issueFields and issueFields.CustomField together.
func (i issueFields) MarshalJSON() ([]byte, error) {
jsonFields := map[string]interface{}{}
if i.Summary != nil {
jsonFields["summary"] = i.Summary
}
if i.Description != nil {
jsonFields["description"] = i.Description
}
to ensure that the field is nil and not an empty string when updating existing issues with the fields not being included. parsing an empty string would just update the field to an empty string so we have to make sure it´s omitted from the JSON completely. I´m not quite sure if a string pointer could be used here as well I tested it and I think it would need quite some reworking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We try to avoid using any or interface{} in the configuration file. Instead, we tend to work on the basis that an empty string means no value. Why can't we just do if i.Summary != ""?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I'm inclined to agree here. And if the intent is "a string or nil", I think think using a *string expresses that much better than any. Otherwise, we're opening ourselves up to future changes introducing mistyped fields in jira requests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep! Use *string if you want empty string to be a valid value instead of empty, otherwise use string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
somehow anticipated this change request and I yes I agree with you :-) . Changed it to use *string. Added little helper to the unit test
func stringPtr(v string) *string {
return &v
}
c80b5fa to
14e532a
Compare
|
Built in more improvements
new logic for the atlassian v3 api endpoint. (which uses atlassian document format). I had to rewrite this because of the string pointer anyway so It´s optimized for only check for valid JSON. The actual encoding takes place in the doAPIRequestFullPath function anyway. However, v3 endpoint with atlassian document format is broken atm, but this is another issue I´ll reply to #4585. This needs quite some refactoring, too. |
Spaceman1701
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for making these changes! One small thing in the new logging code, but otherwise LGTM.
ed1337d to
8a7cbdb
Compare
Spaceman1701
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
@SuperQ mentioned one last change he would like: could we have the flag be positive? Thanks! |
sure, however it´s a breaking change then because the default behavior will change from update everything to NOT update summary / description if not explicit set otherwise. is that ok for you? |
What about |
|
Yes, my suggestion was to change the naming / default from This avoids the |
I see. But since it´s a bool and defaults to false how exactly you want to make it default to true? |
|
Take a look at the Prometheus https://github.com/prometheus/prometheus/blob/main/config/config.go#L192-L200 |
d0bf3b0 to
d2dd0cf
Compare
Signed-off-by: Holger Waschke <[email protected]>
Signed-off-by: Holger Waschke <[email protected]>
491124b to
16a4342
Compare
Played around a bit and came up with this solution. |
This change allows disabling the update of issue descriptions.
It´s a non breaking change the default is still to update everything. Only if you set the parameter disable_update_description explicit to true the description won´t get updated anymore.
We need this feature as there a lot of jira automations which updates the description itself, jiralert would revert the changes back to the original templated description state once the repeat_interval kicks in and the alert is still active, so it´s handy to have the possibility to disable it.
I added a new unit test for this function to test.
Exempel config